Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support for snapshot 24w04a (1.20.5) #3602

Closed
wants to merge 15 commits into from

Conversation

Outfluencer
Copy link
Collaborator

@Outfluencer Outfluencer commented Jan 20, 2024

related to #3601
if you spot bugs please report

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Jan 20, 2024

thoughts: maybe add transfer api and cookie api, also maybe provide config options to decline transfers

@Outfluencer Outfluencer changed the title support for snapshot 24w03a (1.20.5) support for snapshot 24w03b (1.20.5) Jan 20, 2024
@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Jan 20, 2024

Notice: if you want to test with an vanilla server (i think there are no spigot snapshot build) you need to change the uniqueId in
handle(final EncryptionResponse encryptResponse)
to the offline uuid

Callback<String> handler = new Callback<String>()
{
    @Override
    public void done(String result, Throwable error)
    {
        if ( error == null )
        {
            LoginResult obj = BungeeCord.getInstance().gson.fromJson( result, LoginResult.class );
            if ( obj != null && obj.getId() != null )
            {
                loginProfile = obj;
                name = obj.getName();
                uniqueId = Util.getUUID( obj.getId() );
                // here is the change
                if ( VANILLA_TESTING )
                {
                    // otherwise we get kicked for invalid signature on vanilla
                    uniqueId = UUID.nameUUIDFromBytes( ( "OfflinePlayer:" + getLoginRequest().getData() ).getBytes() );
                }
                finish();
                return;
            }
            disconnect( bungee.getTranslation( "offline_mode_player" ) );
        } else
        {
            disconnect( bungee.getTranslation( "mojang_fail" ) );
            bungee.getLogger().log( Level.SEVERE, "Error authenticating " + getName() + " with minecraft.net", error );
        }
    }
};

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Jan 20, 2024

@md-5 i thought about the transfer api, should we store a cookie inside the client like "bungeecord:transfer" with a json as data like the BungeeCord version and build number and a timestamp, before we sent the transfer packet to the player?

Whats your opinion on this one?
Do you have any idea for Cookie standard format, that may also be used in spigot

@OpticFusion1
Copy link

@md-5 i thought about the transfer api, should we store a cookie inside the client like "bungeecord:transfer" with a json as data like the BungeeCord version and build number and a timestamp, before we sent the transfer packet to the player?

Whats your opinion on this one?

I personally see no reason for this.

@NEZNAMY
Copy link

NEZNAMY commented Jan 20, 2024

Using sendPacket may cause the packet to be sent during wrong phase (such as configuration).

@Outfluencer
Copy link
Collaborator Author

Using sendPacket may cause the packet to be sent during wrong phase (such as configuration).

what exactly do you mean?

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Jan 20, 2024

the cookie packets are present in game login and config phase and the transfer packet witch api is only after login state is registered for configuration and game phase, its not possible that the packet is in the wrong phase as its registered for all phases that can occour

@md-5
Copy link
Member

md-5 commented Jan 22, 2024

Notice: if you want to test with an vanilla server (i think there are no spigot snapshot build) you need to change the uniqueId in handle(final EncryptionResponse encryptResponse) to the offline uuid

Callback<String> handler = new Callback<String>()
{
    @Override
    public void done(String result, Throwable error)
    {
        if ( error == null )
        {
            LoginResult obj = BungeeCord.getInstance().gson.fromJson( result, LoginResult.class );
            if ( obj != null && obj.getId() != null )
            {
                loginProfile = obj;
                name = obj.getName();
                uniqueId = Util.getUUID( obj.getId() );
                // here is the change
                if ( VANILLA_TESTING )
                {
                    // otherwise we get kicked for invalid signature on vanilla
                    uniqueId = UUID.nameUUIDFromBytes( ( "OfflinePlayer:" + getLoginRequest().getData() ).getBytes() );
                }
                finish();
                return;
            }
            disconnect( bungee.getTranslation( "offline_mode_player" ) );
        } else
        {
            disconnect( bungee.getTranslation( "mojang_fail" ) );
            bungee.getLogger().log( Level.SEVERE, "Error authenticating " + getName() + " with minecraft.net", error );
        }
    }
};

I guess this only applies if bungee is online mode? Should probably look into this and if it can be supported without an option as Bungee should support vanilla.

@md-5
Copy link
Member

md-5 commented Jan 22, 2024

@md-5 i thought about the transfer api, should we store a cookie inside the client like "bungeecord:transfer" with a json as data like the BungeeCord version and build number and a timestamp, before we sent the transfer packet to the player?

Whats your opinion on this one? Do you have any idea for Cookie standard format, that may also be used in spigot

I don't think that's necessary, but there should be an API

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Jan 22, 2024

Notice: if you want to test with an vanilla server (i think there are no spigot snapshot build) you need to change the uniqueId in handle(final EncryptionResponse encryptResponse) to the offline uuid

I guess this only applies if bungee is online mode? Should probably look into this and if it can be supported without an option as Bungee should support vanilla.

Yes only for online mode, for offline mode it works fine as the uuids are the same, in online mode i think some signatures are broken in some packets because of the uuid difference

@Outfluencer
Copy link
Collaborator Author

@md-5 you can achieve vanilla compatibility by dropping the Chat Session packet if enforce secure profile is set to false in bungee, if you do, you also can send messages to the server, but when the message is sent from the server to all clients they will show this:
image

@md-5
Copy link
Member

md-5 commented Jan 22, 2024

@md-5 you can achieve vanilla compatibility by dropping the Chat Session packet if enforce secure profile is set to false in bungee, if you do, you also can send messages to the server, but when the message is sent from the server to all clients they will show this: image

If we do the fix you initially mentioned does the chat error show?

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Jan 23, 2024

@md-5 you can achieve vanilla compatibility by dropping the Chat Session packet if enforce secure profile is set to false in bungee, if you do, you also can send messages to the server, but when the message is sent from the server to all clients they will show this: image

If we do the fix you initially mentioned does the chat error show?

as i remember it does not show

@md-5
Copy link
Member

md-5 commented Jan 23, 2024

@md-5 you can achieve vanilla compatibility by dropping the Chat Session packet if enforce secure profile is set to false in bungee, if you do, you also can send messages to the server, but when the message is sent from the server to all clients they will show this: image

If we do the fix you initially mentioned does the chat error show?

as i remember it does not show

Can you please check? And then perhaps make a separate PR but replace VANILLA_TESTING with (onlineMode && !ipForward)?

@Outfluencer
Copy link
Collaborator Author

Okay i can do it when i am home in like 2 hours

@md-5
Copy link
Member

md-5 commented Jan 23, 2024

No rush

@Outfluencer
Copy link
Collaborator Author

Outfluencer commented Jan 23, 2024

nah i forget i got my laptop on me, i tested, its working

@Outfluencer Outfluencer changed the title support for snapshot 24w03b (1.20.5) support for snapshot 24w04a (1.20.5) Jan 24, 2024
@md-5
Copy link
Member

md-5 commented Jan 24, 2024

Based on an initial review, looks good to me. Well done!

@Outfluencer
Copy link
Collaborator Author

What do you guys think about adding an Player Transfer Event, so we can cancel transfers by the backend server?

@md-5 md-5 self-assigned this Jan 29, 2024
Copy link
Member

@md-5 md-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks really good. Some minor comments.

@Override
public void storeCookie(String cookie, byte[] data)
{
Preconditions.checkState( getPendingConnection().getVersion() >= ProtocolConstants.MINECRAFT_1_20_5, "Cookies are only supported in 1.20.5 and above" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to discuss - whether we should force a namespace

Copy link
Collaborator Author

@Outfluencer Outfluencer Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for storeCookie, we don't need to as the client creates a "minecraft:" namespace if no namespace is sent, thats why i added "minecraft:" as default namespace for retrieveCookie so we can run code like this without any issues, because for the CookieRequest it doesn't do that

player.storeCookie("test", "Outfluencer".getBytes());
player.retrieveCookie("test").thenAccept((data) -> {
    player.sendMessage(new String(data));
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I know. But maybe for good API we want to force a namespace

@md-5
Copy link
Member

md-5 commented Jan 29, 2024

Thanks!

FYI I made some minor changes to the cookie handling in InitialHandler to make it properly thread safe given it's a future and may commonly be used async. Please let me know if any concerns.

@md-5 md-5 closed this Jan 29, 2024
md-5 pushed a commit that referenced this pull request Jan 29, 2024
@md-5
Copy link
Member

md-5 commented Feb 2, 2024

Should we add an isTransferred to PendingConnection ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants